fix: Import cloud-backed PKPASS files off the main thread - #3282
fix: Import cloud-backed PKPASS files off the main thread#3282mvanhorn wants to merge 4 commits into
Conversation
|
I guess this makes some sense but I have some notes:
|
…failure toasts - Route image/PDF/pkpass/espass imports through one background importFile - Restore errorReadingFile / errorReadingImage / noBarcodeFound toasts via a main-looper-marshalling showToast helper - Replace fixed-timeout awaits in the activity tests with a waitFor predicate
…read launch(Dispatchers.IO) starts the coroutine on a background thread immediately, so a paused Robolectric main looper never drives it and the activity tests time out. Launch on the lifecycle scope's default (main) dispatcher and wrap only the blocking read in withContext(Dispatchers.IO).
Two separate bugs, both in the tests I added: - new Intent(ACTION_VIEW, uri).setType(...) clears the data URI (setType and setData clear each other), so the activity received a null Uri and the read never started. Use setDataAndType. - The intermediate-state assertions (no started activity / RESULT_CANCELED) are not observable: waiting on the main looper drains it, and Robolectric drains it again during .visible(), so the import has already run to completion by the time the test regains control. Assert the off-main-thread read directly instead, which is the property these tests exist to prove.
|
All three notes addressed, CI is green.
Unrelated: LoyaltyCardViewActivityTest.startWithLoyaltyCardNoExpirySetExpiry fails locally at a date rollover (expects "August 26" and gets "August 25"). It fails the same way on the base branch without my changes, so it is not from this PR, but you may want to pin a clock in that test. |
TheLastProject
left a comment
TheLastProject
left a comment
There was a problem hiding this comment.
Seems to work. I will admit the tests are a bit beyond me, but otherwise this makes sense: we're moving it off the main thread and we also stop seeing the NetworkOnMainThread exception. Quite nice. That I can't really follow the tests well is not a huge stopper for me as I tested it manually and can confirm it works.
I have an opinion on showToast, but that's mostly it.
I did notice something interesting though: when using a pkpasses file (I used the Eurowings one in the tests directory), it fails only in the scan activity. It seems to be routing it wrong there. I don't think this is part of your MR, but may be interesting to look at. If you consider it out of scope I'll make a new issue for that. I do think it'd be good to fix that up before doing a new release though, just to prevent "the release said it was fixed" cases.
The cleanest fix for that last bug which I stumbled up by pure chance is probably to add a new BARCODE_IMPORT_FROM_PKPASSES_FILE type to Utils instead of trying to resolve application/vnd.apple.pkpasses (maybe Nextcloud doesn't pass that correctly, who knows).
Error reading pkpass file
java.lang.IllegalStateException: File lacks pass.json
at protect.card_locker.PkpassParser.<init>(PkpassParser.kt:112)
at protect.card_locker.Utils.retrieveBarcodesFromPkPass(Utils.java:201)
at protect.card_locker.Utils.parseSetBarcodeActivityResult(Utils.java:392)
at protect.card_locker.ScanActivity$handleActivityResult$1.invokeSuspend(ScanActivity.kt:331)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:34)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:101)
at kotlinx.coroutines.internal.LimitedDispatcher$Worker.run(LimitedDispatcher.kt:113)
at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:89)
at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:589)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:823)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:720)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:707)
| private static void showToast(Context context, int message) { | ||
| if (Looper.myLooper() == Looper.getMainLooper()) { | ||
| Toast.makeText(context, message, Toast.LENGTH_LONG).show(); | ||
| } else { | ||
| new Handler(Looper.getMainLooper()).post( | ||
| () -> Toast.makeText(context, message, Toast.LENGTH_LONG).show()); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it'd be better if you could pass the toast length to this function, so that it is reusable also for short toasts in the future.
And adding a comment to this function to explain what it does, something like "displays toast instantly if on the main UI loop, otherwise ask Android to show it on the UI loop" would help clarity on why this function exists.
Also, all other functions seem to be static private instead of private static. Probably good to order it the same for easier skimming of the code.
Yup, this is a known bug. See #2124. I think properly allowing to set users to set a time should probably fix those tests too, see #2284. |
2 participants
Keep
PkpassParser,PkpassesParser, and the existing synchronous parsing utilities as the parsing boundary, but invoke PKPASS and PKPASSES work from lifecycle-scoped coroutines onDispatchers.IOin both production entry points. PKPASS imports opened through a cloud-backed Android document provider can require network access whileContentResolver.openInputStreamis resolving the URI.Opening a valid
.pkpassthroughMainActivitywith a content stream that records its caller reads the provider stream off the main looper, then processes the parsed result on the main thread; Returning a valid.pkpassfromScanActivity's picker reads and parses off the main looper, then delivers the selected result while UI/scanner state changes remain on the main thread.Fixes #2464